Materialization audit, Elle cert hardening, formation election-stall fix, replayable Accord log - #308
Materialization audit, Elle cert hardening, formation election-stall fix, replayable Accord log#308bkearns wants to merge 16 commits into
Conversation
…t (t_2487eeb7) The audited-crate set was a hand-maintained list, which made OOM-guard coverage opt-in — and opt-in coverage rots. `ferrosa-graph` and `ferrosa-sparql` both grew query-sized serving paths, and shipped real materialization OOMs, while sitting entirely outside the gate. The audit reported "no findings" the whole time. Coverage is now exhaustive: every workspace member must be either audited or listed in NON_SERVING_CRATES with the reason it owns no query-sized path. A crate in neither set produces an `unclassified-crate` finding, so adding a crate fails the audit instead of silently widening the blind spot. The check runs in the binary rather than only in a unit test, and an unreadable workspace manifest is itself a finding — unknown coverage must never read as a clean run. Also teaches `returns-vec-partition-or-row` about `Vec<VirtualRow>`. `VirtualTable::read` returns a whole table while `visit_rows` streams, and the trait docs already tell large/live tables to override the visitor; this is what holds them to it. Audited crates: 6 -> 15. Findings: 0 -> 82, all pre-existing materialization the gate could not previously see. 21 of them are in crates it was ALREADY auditing, hidden purely by the missing VirtualRow shape. One is confirmed class A (t_250fa355: read_anchor_partitions range_read arm, which its own doc comment already called "the unbounded materialization"). The 82 are baselined in oom-audit-allow.toml marked NOT INDIVIDUALLY TRIAGED, with a short expiry (2026-10-01) so they fail the gate rather than becoming a permanent rubber stamp. The reasons deliberately do not claim these sites are safe. Triage is t_a49d88c3. Known gap: entries omitting `symbol` suppress a whole (file, rule) pair, so a new violation inside an already-allowlisted file is not caught (verified). Tracked as t_e1d5f83c. Verified: enforce exits 0 today and 1 after the baseline expiry; a new materializing fn in a newly-audited crate is caught (exit 1).
…findings The t_2487eeb7 baseline marked all 52 entries "NOT individually triaged". That was true when written; it is no longer true for ferrosa-graph and ferrosa-sparql, and an allowlist that understates what is known is as misleading as one that overstates it. 14 entries now carry the actual bound: schema-arity with_capacity, single-row clones, max_result_rows-bounded copies, the fail-loud max_message_size guard, and two genuine rule misfires (.cloned() over an Option; SET/REMOVE loops whose push targets a per-column vec, not the stream). 5 entries are re-marked CONFIRMED REAL rather than merely untriaged — the range_read call sites in expand.rs, leapfrog.rs, varpath.rs and reconcile.rs. Their expiry moves in to 2026-09-15 so known bugs surface before unknown ones. Root cause and fix plan: t_bc5f0e6f (WritePath::range_read is a materializing wrapper over an already-existing stream, so each is an in-place swap). Notable corrections to earlier belief: - sparql DEFAULT_MAX_ROWS is NOT a silent result cap. Crossing it raises SparqlError::Execution — a loud error, never a truncation. It prevents OOM rather than causing it, so the entry records that instead of implying a debt. - The Bolt codec with_capacity sites are bounded for query data, but the count is wire-declared and decoded pre-auth. Filed separately as t_3e665492 (allocation amplification), since it is a different failure mode than materializing query results. 33 entries in the remaining crates stay honestly marked untriaged (t_a49d88c3). Verified: enforce exits 0 today; the confirmed-real entries fire as expired on 2026-09-16; 53 xtask tests green.
…s its bound Finishes what 8e08f17 started. All 52 baseline entries now carry a real classification: 44 TRIAGED (bounded or rule misfire, each citing the specific bound) and 9 CONFIRMED REAL (expiring 2026-09-15, ahead of the rest). Zero entries remain marked "NOT individually triaged". Three structural patterns explain nearly all of the 47 newly-classified findings: - Virtual tables: every read() collects an in-memory registry into Vec<VirtualRow> while the streaming twin visit_rows exists. The class is the REGISTRY's bound, not the read's — connections, peers, in-flight queries, alert rules, MVs, tables and indexes are all server-resource bounded. - ferrosa-sql is a materializing executor whose blocking operators cannot be streamed at all (t_50d99192). - sstable/postgres/flight/view are bounded by schema arity, one flush-sized SSTable, or one Arrow RecordBatch. TWO SUSPECTED class-A findings REFUTED by reading the code rather than trusting the pattern: - billing.rs buckets looked unbounded (tenant x keyspace x minute) but are capped by MAX_BUCKETS with evict_oldest() at :79-80. - query_fingerprints.rs looked attacker-amplifiable via distinct query shapes but is capped at MAX_ENTRIES=10_000 with evict_lowest() at :74-75. Both drop to bounded. Class-A count is 19, not the 21 first reported. Two rule misfires are worth recording because they are INVERTED — the flagged constants are memory-BOUNDING, the opposite of the risk the rule describes: PARTITION_TOKEN_SUMMARY_MAX_ENTRIES downsamples so reader memory does not scale with table size, and MAX_NODE_READ is a fixed single-trie-node buffer. New fix tasks: t_f348ba0b (postgres full-table SELECT materialization, client-facing, highest), t_50d99192 (ferrosa-sql executor memory bound). Verified: enforce exits 0 today; confirmed-real entries fire as expired on 2026-09-16; 53 xtask tests green.
4b8fbf0 to
60af610
Compare
|
Reviewer coverage statement (honest scope, per-commit): Deep line review (by me, this cycle):
Reviewed as a set (Elle hardening, 6 commits Pre-existing from original PR scope (not re-reviewed this cycle): the materialization-audit/xtask, Accord framed-log/crash-recovery, and postgres offload commits predate this convergence effort and rode through the rebase unchanged (3-way, no content conflicts outside the graph repair noted above). |
… (t_d3b2dec1) `ferrosa_sql::execute` is synchronous and CPU-bound — scan, filter, sort, hash-aggregate, hash-join over a materialized row set — and both PG query paths called it directly inside an async handler (query.rs:705 simple, server.rs:654 extended). There was no spawn_blocking anywhere in ferrosa-postgres, so one PG-wire SELECT over a large table occupied an async worker for the whole sort/join. That is the shape PR #131 fixed on the CQL keepalive path, where sync work inline on an async worker starved keepalives and raft heartbeats into a "pool is broken" / CheckQuorum step-down. Same mechanism, heavier per call. Adds `offload::execute_offloaded`, which moves the statement, catalog, schema and params onto a blocking thread. The move is cheap: `MapCatalog`'s tables are `Arc<dyn TableProvider + Send + Sync>`, so it is a refcount bump, not a copy of row data. Why spawn_blocking and not ferrosa_sched: `submit_scan` is built for chunked streaming producers — it hands back a `ScanSlot` to yield against and wires channel-close cancellation. A relational `execute` is one opaque call with no yield points, so it has nothing to do with a `ScanSlot`. spawn_blocking is not unbounded here either: the data and background runtimes set explicit max_blocking_threads ceilings (t_88223ad0) so blocking work cannot oversubscribe the cores and starve consensus. A panic in the executor is re-raised with its original payload rather than flattened into an `ExecError`. Every `ExecError` variant is a user-facing SQL condition; a panic is an engine bug, and dressing it up as a query error would hide a defect behind a message the client cannot act on. TESTS: `executor_does_not_run_on_the_async_worker` observes the executor's thread through a `Catalog::resolve` that records `thread::current().id()` — resolve runs inside `execute`, so it reports the thread the executor actually ran on. Verified the test has teeth by temporarily inlining the call: it fails with "the synchronous executor ran on the async worker". A second test asserts offloading is behavior-preserving, error case included. This is also the prerequisite for t_f348ba0b: bridging the async storage stream into TableProvider::scan's synchronous Iterator needs a bounded channel whose consumer may block on recv, which is only safe off the runtime. ferrosa-postgres 139 tests green, clippy clean, fmt clean.
The dispatch arm for BEGIN/COMMIT/ROLLBACK said transactions were not yet implemented. They are: server::execute_simple intercepts them and drives the session's buffered write-set through Accord (commit_txn), and the extended protocol does the same — FMEA PG-1 records write atomicity as Implemented over both protocols. That arm is only reachable by a caller that enters dispatch directly with no session state, so it now says what is actually true: transaction control needs a session. A reader following the old message would have concluded ferrosa-postgres has no transaction support at all, which is wrong and would misdirect anyone scoping the Elle certification work (t_27672375).
…ON.md The note predated the CI job. `.github/workflows/elle-strict-serializable-nightly.yml` exists and runs both the fault-free and nemesis certifications nightly against a 3-node RF=3 compose cluster, opening an issue on failure. Understating coverage is its own hazard: a reader scoping the Postgres certification (t_27672375) would have concluded Elle has no regression guard and rebuilt one. Records the division of labour — CI covers regression, the Fly run remains the manual reference certification on real multi-host infrastructure.
…bcc) Every existing nemesis fault is a NETWORK fault — isolate, isolate-coordinator, netem latency. None of them ever discards local state, so the passing strict-serializability certification says nothing about process death. That gap matters here specifically. Nothing replays the Accord protocol log at startup, so a restarted node returns with an EMPTY ConflictIndex — no memory of in-flight transactions — and begins serving PreAccept immediately. A replica answers PreAccept with the dependencies it knows about, so an amnesiac replica reports an INCOMPLETE dep set, which is the shape that produces real-time ordering anomalies (:strong-PL-1-cycle-exists / :G-nonadjacent-item-realtime). Whether quorum covers that, or whether a restarted node must be fenced until recovery completes, is unanswered — and untested. Adds `inject_restart_replica`: SIGKILL node3, restart it, wait for it to serve CQL again. Design notes worth keeping: - SIGKILL rather than a graceful restart. A clean shutdown could flush state a real crash would not, testing the easy case and hiding the one we care about. - node3, NOT the seed. The generator runs ON node1 and writes its history to node1's /tmp; restarting node1 would kill the generator mid-run and take the history with it. A non-coordinator restart still exercises the amnesia question. Coordinator restart needs the generator off-cluster and is separate. - The fault verifies it BIT (machine state after SIGKILL) and that node3 REJOINS and serves CQL. A node that never rejoins silently degrades the run to two nodes, which is a weaker test, not a pass — so that case logs a loud warning telling the reader to distrust the verdict. Either outcome is worth having: green converts an assumption into evidence; red finds a real consensus bug in a path nothing currently covers. The likely fix if red is fencing, NOT storing write keys in the protocol log (see t_4b91be79 for why that lever was rejected). Harness change only — no certification run performed.
…f its target Extends the restart fault (20dbc80c) to cover the COORDINATOR, which is the more interesting case: the coordinator holds the in-flight Accord state for the transactions it is driving, so its amnesia on restart is what most plausibly produces an incomplete dep set. That required moving the generator. It previously ran ON the seed against `localhost:9042`, which cannot survive a coordinator restart — SIGKILLing that machine would destroy the generator process AND the history in its /tmp. The generator now runs on node2 (never restarted) and drives node1 over the internal network, so node1 can be killed while the run and its evidence continue. This is safe by construction: each worker is pinned to one target with NO retry failover, and a failed BEGIN/UPDATE classifies `:fail` while a failed COMMIT classifies `:info` — never `:fail` — so a write that did land cannot masquerade as one that did not. Verified in elle_list_append.rs before relying on it. Two guards against a falsely-green run: - A generator preflight (node2 -> node1:9042) fails BEFORE the ~40min build+run rather than producing an empty history from a bad address. - The schedule roughly doubled in length, because each restart blocks until the node serves CQL again. A fault that fires after the generator has finished hits an IDLE cluster and proves nothing, so GEN_ARGS is scaled up (8/1800/8/3) and the script now REPORTS whether the generator actually outlasted the schedule. An under-provisioned run is visible instead of silently degrading the new faults into no-ops while still printing `valid? true`. Harness only — no certification run performed yet.
…ed quotes The first run of the restart-fault cert aborted at the generator preflight with GEN_FAIL. That was not a real connectivity failure — both of my new checks were broken, in two independent ways: 1. `/dev/tcp/host/port` is a BASH builtin. `nem` runs `sh -lc`, and /bin/sh on debian:trixie-slim is dash, which has no /dev/tcp — so the test could only ever fail, regardless of whether the path was reachable. 2. `nem` wraps its argument in single quotes (`sh -lc '$2'`), so the nested single quotes in `sh -c '</dev/tcp/...'` terminated the string early and mangled the command. Both checks now use curl, which is installed in the image and is already the readiness probe `wait_ready` trusts, with no nested quotes. The preflight targets node1's WEB port rather than raw CQL: CQL binds `[::]:9042` with the same wildcard as web's `[::]:9090`, so a reachable web port proves the DNS + cross-node IPv6 path the generator needs. THE MORE SERIOUS HALF was the rejoin check inside `inject_restart`, which had the identical defect. It would have reported "did NOT rejoin within 300s" for BOTH restart faults no matter what actually happened — turning the guard that exists to detect a degraded 2-node run into a permanent false alarm, and undermining exactly the evidence these faults were added to produce. The preflight caught this before the workload ran: the run aborted and tore down cleanly (machines, app, and bucket all destroyed) rather than producing an empty history. That is the guard working as designed — it just happened to fire on its own bug first.
…nate faults TWO CORRECTIONS TO THE PREVIOUS RUN. 1. THE REJOIN TIMING WAS NOT A MEASUREMENT. The loop reported `i*10`, counting iterations while ignoring the several seconds each `flyctl ssh` round-trip costs. So it under-reported real latency, AND its 10s granularity made two restarts appear to land on an identical figure — which I then read as "matches formation_timeout exactly". That inference was not supported by the instrument. Now measured as real elapsed wall-clock and logged as REJOIN_SECONDS alongside the configured timeout, so the correlation can be judged instead of guessed. Poll interval tightened to 5s. 2. THE WORKLOAD WAS TOO SMALL TO MEAN ANYTHING. The run produced 1026 :ok / 5 :info / 13369 :fail — 93% failure — and Elle returned `valid? true` over roughly a thousand successful appends. That is a VACUOUS pass: no anomalies found because almost nothing succeeded, not because ordering held. (The repo has hit this before: a 2026-07-20 run was green on a mostly-rejected-write history and was correctly discarded as weak.) Cause is structural, not a product bug. Each worker pins to one target with no failover (deliberate — a transaction must not split across nodes), that target is node1, and node1 is SIGKILLed for ~2 minutes. Every op in that window fails. Two restarts plus the coordinator isolation account for ~11.5k of the 13.4k failures at the observed ~41 ops/sec. Fixed by sizing the run so the successful portion dominates: GEN_ARGS 8/8000/8/3 (~64k ops, ~26min) against a schedule of roughly 475s. The outlast guard added with the restart faults did its job here — it reported that the generator finished at 349s while the schedule was still running, which is what revealed the coordinator restart had no post-rejoin traffic. Keeping that guard honest is what makes a future green verdict worth citing.
…cross nodes Three cert runs produced vacuous verdicts. Root cause found in the generator, not the database. === 1. RECONNECT WAS GATED ON THE WRONG CONDITION === The session replacement lived inside `if let Some(id) = txn_id_opt`, so a broken session was only ever replaced when BEGIN had already SUCCEEDED. That is exactly backwards: when the connection dies, BEGIN is the statement that fails, so there is no txn id, the branch is skipped, and the worker spins its entire remaining op budget against a corpse. Measured on run 3: 38,543 of 61,564 failures were "BEGIN: No connections in the pool: The pool is broken". One network disruption ~100s in permanently killed all eight workers, leaving Elle to check a history that was 96% connection errors — 2,418 :ok out of 64,000 ops. `valid? true` on that is meaningless. Reconnect is now driven by session HEALTH via `is_session_dead`, independent of which statement failed. Deliberately narrow: pool/connection errors replace the session, while server-side rejections (invalid query, insufficient replicas) leave it alone, since needlessly rebuilding a session would discard the transaction state the pinned-connection model depends on. The READ path had the same defect AND swallowed its error entirely (`Err(_)`), so read failures never reached the tally that exists to explain a run. Both fixed. Why earlier certifications never hit this: the generator ran ON the coordinator against localhost, which survives peer-level ip6tables rules, so the pool never broke. Any off-node generator — required to fault-inject the coordinator itself — depends on this reconnect. === 2. ALL WORKERS PINNED TO ONE NODE === `min_by_key(host_id)` gave every worker the same target, so one node coordinated 100% of the workload. Workers now round-robin across all nodes (schema setup stays on a single session; each worker remains pinned to ITS node, preserving the one-transaction-one-connection invariant that FallthroughRetryPolicy protects). Two things this fixes: killing a node no longer removes all traffic, so a rejoining node faces real load — the condition the restart faults need; and transactions are now coordinated by every node, so cross-coordinator Accord ordering is exercised, which single-target runs never covered. Corrects a wrong diagnosis of my own along the way: I had claimed the generator was dying with its target, on the strength of two runs ending at similar wall-clock despite a 4.4x op count. The 64,000 :invoke count refutes that — the generator completed every op. Failing ops are simply fast, so a mostly-failing run races through its budget.
Local end-to-end validation of the reconnect fix was attempted and could NOT be completed — recorded here so the gap is not mistaken for coverage. The local 3-node compose cluster runs an 8-day-old nightly image that predates server-minted transaction ids, so every append is rejected instantly with "nested transactions are not supported" (15,997 of 16,000 failures). With no network round-trip, 24,000 ops complete in SEVEN SECONDS, so no mid-run node kill can land while the generator is alive. Validating the reconnect end-to-end needs an image built from current main. What the attempt DID verify: the worker-spreading fix works — the generator reports "pinning 6 workers across 3 node(s)" against the live cluster. So the classification that drives the reconnect is unit-tested instead, which needs no cluster: - the five failure texts that actually dominated the fault-injected run are classified dead (they were the 38,543 "pool is broken" BEGIN failures that previously never triggered a reconnect); - server-side rejections — insufficient replicas, invalid query, client timeout, internal error — are classified LIVE. Reconnecting on those would be actively harmful: rebuilding a session mid-run discards the transaction state the pinned-connection model depends on; - classification is case-insensitive, since driver error casing is not a stable contract. Verified the tests bite by inverting `is_session_dead` to always return false: 2 of 3 fail with "must replace the session for: BEGIN: No connections in the pool...". Restored. This tests the DECISION, not the reconnect wiring. The end-to-end path still needs a current-image run before the next certification is trusted.
…aft state timeline The formation test installed no tracing subscriber, so a CI failure printed only the final metrics line. Worse, RUST_LOG=openraft=debug HIDES the formation race (20/20 pass with it, ~3% failures without): synchronous formatted writes on the raft path shift the timing window. So the durable diagnostic is a metrics-watch timeline (25ms sampling, transitions only, one shared epoch across nodes), dumped on failure alongside voters/learners membership — the fields that separate 'votes rejected' from 'elections stopped' (t_b0aac0d3).
…ed for the pre-vote stall) candidate_re_campaigns_while_peers_are_down: the seed initializes a 3-voter membership with both peers down and must keep firing elections — current_term advancing past 1 is the oracle (election-timer liveness, not leadership; a lone seed can never lead a 3-voter cluster). Fails on the unfixed tree with term frozen at exactly 1: the fork's tick election path gates elect() behind a pre-vote round that FerrosRaftNetwork provides no transport for, so every probe is an automatic NO and no election after initialize()'s can ever fire (t_b0aac0d3).
…exists raft_enable_pre_vote defaulted true per ADR-012, but the pinned openraft fork's tick election path hard-gates elect() behind run_pre_vote_round() while FerrosRaftNetwork never overrides pre_vote — the default trait impl returns an unimplemented NetworkError counted as a NO vote. A pre-vote quorum was structurally impossible, so after the seed's single initialize()-driven election at term 1, no election could ever fire again: a transient first-round vote loss became a permanent formation stall (~3% under CPU starvation, CI job 89743146291). Default the gate OFF so the tick path elects directly and a lost round self-heals on the next timeout, exactly like upstream. The FERROSA_RAFT_ENABLE_PRE_VOTE override remains for when the transport lands (t_32cb5ad3). ADR-012 assert inverted with rationale; election-storm mitigation remains the election_guard watchdog, now documented as load-bearing (CL-2/CL-15). Spec: specs/implemented/bug-cluster-formation-pre-vote-election-stall.md (t_b0aac0d3).
60af610 to
40bc1c3
Compare
|
Restructured per owner review: the two blocking incomplete features are SPLIT OUT — graph cap-removal (finding 2) now lives on feat/graph-capless-collect (stacked on this branch; lands with/after the streaming-executor work, t_3f2f961a), and the Accord framed-log + orphan-resolution commits (finding 3) on feat/accord-framed-log-recovery (returns when migration + startup replay wiring + kill/restart test are complete, t_6691c002). #308 is now 16 commits: materialization-lint coverage + triage docs, postgres executor offload, Elle certification hardening, formation diagnosability, and the pre-vote election-stall fix. The p0-oom-audit passes in enforce mode on the slimmed branch (the result-bound rule change rides with its graph fix on the split branch). Nemesis-effectiveness finding (5) tracked as t_2a37f1bf against the certification harness — the harness commits remain here, the fatality hardening lands as a follow-up before the next certification claim. Local gates re-run: clippy 0, cargo doc -D warnings 0, formation suite 4/4. |
Scope grew substantially since opening — this PR now carries four related workstreams, rebased onto e6c768c:
1. Materialization audit coverage + replayable Accord protocol log (original scope)
Exhaustive-by-default materialization lint (NON_SERVING_CRATES + unclassified-crate findings), graph truncation fix (capless
collect_to_graph_result— a mid-drain cap silently truncates with no partial-result flag; bounds belong upstream in the stream), framed Accord protocol log + crash recovery.2. Elle certification hardening (6 commits)
The 2026-07-26 nemesis cert was vacuous: the generator pinned all workers to one node, so killing the coordinator stopped 100% of traffic (1026 :ok / 13369 :fail,
valid? trueover ~1000 single-coordinator appends). Fixes: workers spread across nodes (each pinned to its node, preserving the one-txn-one-connection invariant), dead sessions replaced on any failure, rejoin latency measured properly, workload sized to dominate faults, coordinator-restart fault added, + a local podman certification harness (deploy/local-elle/) so harness breakage is caught before a cloud run.3. Formation diagnosability (2 commits)
The formation test installed no tracing subscriber (CI failures printed nothing usable), and
RUST_LOG=openraft=debugHIDES the formation race (20/20 pass with it) — so the durable diagnostic is a metrics-watch timeline recorder (25ms sampling, shared epoch, voters/learners dump on failure).4. Formation election-stall fix (2 commits: red test + fix) — the bug that failed this PR's own Test + Coverage
Root cause (t_b0aac0d3):
raft_enable_pre_votedefaulted true, but the pinned openraft fork hard-gates the tick election path behind a pre-vote round whose transportFerrosRaftNetworknever implements — every probe is an automatic NO, so after the seed's singleinitialize()-driven election at term 1, no election can ever fire again. A transient first-round vote loss became a permanent stall (~3% under CPU starvation; CI job 89743146291 was exactly this).Fix: default the gate OFF until the transport lands (t_32cb5ad3); tick path then re-elects and self-heals, like upstream. Regression guard:
candidate_re_campaigns_while_peers_are_down(term must advance past 1 with peers down; deterministic red on the unfixed tree).Verification: 250 consecutive CPU-starved formation runs, zero failures (two runs of 200+50, both externally killed mid-run by machine pressure, zero failures in every completed iteration), vs pristine main's measured ~3% (1/30 control + 5/85 across instrumented runs).
Spec:
specs/implemented/bug-cluster-formation-pre-vote-election-stall.md. FMEA updated: CL-2 no longer claims PreVote as a live mitigation (the election_guard watchdog is load-bearing); new CL-15 row.Pre-existing red-check note for reviewers: Test + Coverage may still hit two OTHER catalogued load-sensitive families unrelated to this diff — compaction bounded-wait (t_283f37df) and fair_admit share-capture (t_ffb53cf9). Attribute against those before assuming this PR.